[4376] Add support for URLs to specify a selection of elements in the workbench #4461

flatombe · 2025-01-23T16:22:35Z

Can you please review this PR and let me know:

If the implementation looks OK
What kind of tests do you expect for this feature?

Note: there are 2 small remaining issues I found while testing:

Selecting a representation leads to an empty selection
This is an existing issue which impacts us a bit. I believe it's out of the scope of this PR as it already existed and is not blocking, but it could be good to have a dedicated issue to track this.
To reproduce:
- Start from a diagram with a selection of elements which are displayed
  in the diagram (they can be selected either in the diagram or through
  the 'Explorer' tree)
- Then select either the diagram representation in the 'Explorer' view, or the diagram background.
- => The selection gets set to the diagram representation (briefly), but quickly evolves into an empty selection => KO
- Notice how this does not occur when starting from a selection of elements that are not shown in the diagram, or from an empty selection.
- I believe the issue is that currently selecting a representation when it is opened 'resets' the selection to an empty one. I think the correct behavior would be to keep the representation in the selection.
Graphical selection in diagram is not performed when resolving a URL with a selection.
This one is a bit more annoying.
To reproduce:
- Open a diagram representation, select an element (from the 'Explorer' view or the diagram) that is represented in the diagram.
- Observe that the corresponding graphical element (e.g. node) is selected graphically because its border is in purple.
- Reload the URL
- Observe that the workbench selection gets set appropriately (the selected element in the 'Explorer' has the pink background)
- But the graphical selection is not performed. => KO.
- Expectation: the graphical node must be selected as well.
- I think the issue stems from the effect in useDiagramSelection that depends on selection. When it is executed for the first time, the diagram does not yet have its nodes and edges so nothing gets selected. Adding a dependency from this effect to the getNodes and getEdges leads to flickering between the current and previous selections.

Pull request template

General purpose

What is the main goal of this pull request?

Project management

Has the pull request been added to the relevant project and milestone? (Only if you know that your work is part of a specific iteration such as the current one)
Have the priority: and pr: labels been added to the pull request? (In case of doubt, start with the labels priority: low and pr: to review later)
Have the relevant issues been added to the pull request?
Have the relevant labels been added to the issues? (area:, difficulty:, type:)
Have the relevant issues been added to the same project and milestone as the pull request?
Has the CHANGELOG.adoc been updated to reference the relevant issues?
Have the relevant API breaks been described in the CHANGELOG.adoc? (Including changes in the GraphQL API)
In case of a change with a visual impact, are there any screenshots in the CHANGELOG.adoc? For example in doc/screenshots/2022.5.0-my-new-feature.png

Architectural decision records (ADR)

Does the title of the commit contributing the ADR start with [doc]?
Are the ADRs mentioned in the relevant section of the CHANGELOG.adoc?

Dependencies

Are the new / upgraded dependencies mentioned in the relevant section of the CHANGELOG.adoc?
Are the new dependencies justified in the CHANGELOG.adoc?

Frontend

This section is not relevant if your contribution does not come with changes to the frontend.

General purpose

Is the code properly tested? (Plain old JavaScript tests for business code and tests based on React Testing Library for the components)

Typing

We need to improve the typing of our code, as such, we require every contribution to come with proper TypeScript typing for both changes contributing new files and those modifying existing files.
Please ensure that the following statements are true for each file created or modified (this may require you to improve code outside of your contribution).

Backend

This section is not relevant if your contribution does not come with changes to the backend.

General purpose

Are all the event handlers tested?
Are the event processor tested?
Is the business code (services) tested?
Are diagram layout changes tested?

Architecture

Are data structure classes properly separated from behavioral classes?
Are all the relevant fields final?
Is any data structure mutable? If so, please write a comment indicating why
Are behavioral classes either stateless or side effect free?

Review

How to test this PR?

Open a project
Select element(s) like documents, semantic elements or representations from the workbench (e.g. in 'Explorer' view or in a representation).
Observe that the URL changes as the workbench selection evolves.
Observe that loading such a URL does set the workbench selection as specified.

Has the Kiwi TCMS test suite been updated with tests for this contribution?

sbegaudeau

The whole code in EditProjectView should be extracted in a dedicated hook

CHANGELOG.adoc

sbegaudeau · 2025-02-03T15:10:47Z

packages/core/frontend/sirius-components-core/src/workbench/Workbench.tsx

+  useEffect(() => {
+    onSelectionChanged(selection);
+  }, [selection]);


This seems very odd, the workbench is retrieving the selection from above and now it need to tell someone above it that the selection has changed. It should be determined above the workbench. I don't think this component has to changed.

I added this here because when defining the same effect in EditProjectView, I was not seeing any changes in the selection, so we never had the chance to react to it.

sbegaudeau · 2025-02-03T15:11:00Z

packages/core/frontend/sirius-components-core/src/workbench/Workbench.types.ts

@@ -56,6 +57,7 @@ export type WorkbenchProps = {
  editingContextId: string;
  initialRepresentationSelected: RepresentationMetadata | null;
  onRepresentationSelected: (representation: RepresentationMetadata | null) => void;
+  onSelectionChanged: (selection: Selection | null) => void;


Same comment

(same as above)

packages/sirius-web/frontend/sirius-web-application/src/views/edit-project/EditProjectView.tsx

sbegaudeau · 2025-02-03T15:13:47Z

packages/sirius-web/frontend/sirius-web-application/src/views/edit-project/EditProjectView.tsx

+  }
+}
+
+function updateUrlSearchParamsWithSelection(setUrlSearchParams: SetURLSearchParams, selection: Selection) {


Don't propagate hook functions like that, just compose your hook

I tweaked a bit my implementation to rely on useSelection() hook, but the URLSearchparams hook requires a Router context because it relies on useLocation hook. Not sure how to work around this otherwise.

packages/sirius-web/frontend/sirius-web-application/src/views/edit-project/EditProjectView.tsx

sbegaudeau · 2025-02-03T15:47:20Z

Selecting a representation leads to an empty selection

Not "a representation" but specifically a diagram, it's a bug in the code of the selection of diagrams

Graphical selection in diagram is not performed when resolving a URL with a selection.

That's an issue, we will have to look into this

mcharfadi · 2025-02-03T16:28:56Z

For the second issue, it should be possible to set the selection in useInitialFitToScreen since this hook guarantees that the nodes are initialized and the code inside this hook is executed only on the first render.

Bug: eclipse-sirius#4376 Signed-off-by: Florent Latombe <[email protected]>

* When the workbench selection changes, the URL search parameters are now automatically updated to encode the contents of the selection (id and kind). * Reversely, when resolving a URL with such search parameters, the workbench selection is set to those specified elements. Bug: eclipse-sirius#4376 Signed-off-by: Florent Latombe <[email protected]>

There was an issue where when resolving a URL with a selection and an open diagram, the selection was not being applied graphically in the diagram because at the time the selection is applied, the diagram exists but has no nodes or edges. This is a workaround to make sure the selection gets applied during the first render *after* the nodes and edges have been created. Bug: eclipse-sirius#4376 Signed-off-by: Florent Latombe <[email protected]>

flatombe requested a review from sbegaudeau as a code owner January 23, 2025 16:22

flatombe marked this pull request as draft January 23, 2025 16:22

flatombe force-pushed the dev/4376 branch from f6a04e0 to 45b3287 Compare January 24, 2025 16:54

flatombe changed the title ~~[4376] Add support for URLs to specify a selection of elements in the workbench (WIP)~~ [4376] Add support for URLs to specify a selection of elements in the workbench Jan 24, 2025

flatombe marked this pull request as ready for review January 24, 2025 16:55

pcdavid linked an issue Jan 27, 2025 that may be closed by this pull request

Take into account all elements of the selection to compute the edit project URL #4376

Open

sbegaudeau added this to the 2025.2.0 milestone Feb 3, 2025

sbegaudeau added pr: to review soon priority: high labels Feb 3, 2025

sbegaudeau requested changes Feb 3, 2025

View reviewed changes

flatombe added 2 commits February 4, 2025 09:36

[4376] Add Workbench prop to apply an effect based on a Selection

a7aaefa

Bug: eclipse-sirius#4376 Signed-off-by: Florent Latombe <[email protected]>

[4376] Make navigation in EditProjectView preserve URL search params

0503568

Bug: eclipse-sirius#4376 Signed-off-by: Florent Latombe <[email protected]>

flatombe marked this pull request as draft February 4, 2025 08:42

flatombe force-pushed the dev/4376 branch 4 times, most recently from c54cb99 to 9fba3e4 Compare February 4, 2025 16:57

flatombe added 2 commits February 4, 2025 18:02

flatombe force-pushed the dev/4376 branch from 9fba3e4 to ba0a1d0 Compare February 4, 2025 17:02

flatombe marked this pull request as ready for review February 4, 2025 17:04

flatombe requested a review from gcoutable as a code owner February 4, 2025 17:04

flatombe requested a review from sbegaudeau February 5, 2025 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4376] Add support for URLs to specify a selection of elements in the workbench #4461

[4376] Add support for URLs to specify a selection of elements in the workbench #4461

flatombe commented Jan 23, 2025 •

edited

Loading

sbegaudeau left a comment

sbegaudeau Feb 3, 2025

flatombe Feb 4, 2025

sbegaudeau Feb 3, 2025

flatombe Feb 5, 2025

sbegaudeau Feb 3, 2025

flatombe Feb 4, 2025

sbegaudeau commented Feb 3, 2025

mcharfadi commented Feb 3, 2025 •

edited

Loading

[4376] Add support for URLs to specify a selection of elements in the workbench #4461

Are you sure you want to change the base?

[4376] Add support for URLs to specify a selection of elements in the workbench #4461

Conversation

flatombe commented Jan 23, 2025 • edited Loading

Pull request template

General purpose

Project management

Architectural decision records (ADR)

Dependencies

Frontend

General purpose

Typing

Backend

General purpose

Architecture

Review

How to test this PR?

sbegaudeau left a comment

Choose a reason for hiding this comment

sbegaudeau Feb 3, 2025

Choose a reason for hiding this comment

flatombe Feb 4, 2025

Choose a reason for hiding this comment

sbegaudeau Feb 3, 2025

Choose a reason for hiding this comment

flatombe Feb 5, 2025

Choose a reason for hiding this comment

sbegaudeau Feb 3, 2025

Choose a reason for hiding this comment

flatombe Feb 4, 2025

Choose a reason for hiding this comment

sbegaudeau commented Feb 3, 2025

mcharfadi commented Feb 3, 2025 • edited Loading

flatombe commented Jan 23, 2025 •

edited

Loading

mcharfadi commented Feb 3, 2025 •

edited

Loading